Skip to content

fix(llm-obs): route experiment create/update through the raw client - #682

Merged
platinummonkey merged 1 commit into
DataDog:mainfrom
tillwf:till.wohlfarth/llm-obs-experiment-writes-raw
Jul 27, 2026
Merged

fix(llm-obs): route experiment create/update through the raw client#682
platinummonkey merged 1 commit into
DataDog:mainfrom
tillwf:till.wohlfarth/llm-obs-experiment-writes-raw

Conversation

@tillwf

@tillwf tillwf commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Problem

experiments create and experiments update both report failure on requests that already succeeded, because the generated typed client rejects the API's actual responses.

experiments update — a successful PATCH /api/v2/llm-obs/v1/experiments/{id} answers HTTP 200 with a zero-byte body. Verified against the live API:

PATCH .../experiments/{id}  ->  HTTP 200, body bytes=0

The generated client deserializes any non-error response unconditionally:

if !local_status.is_client_error() && !local_status.is_server_error() {
    match serde_json::from_str::<LLMObsExperimentResponse>(&local_content) {
        Ok(e)  => ...,
        Err(e) => return Err(datadog::Error::Serde(e)),   // empty body lands here
    }
}

from_str::<T>("") fails with EOF while parsing a value, line 1 column 0, so the command exits 1 after the write has landed. Anything scripting pup reads that exit code as failure and retries — double-writing. I hit this four times in one session; every write had applied.

experiments create — the API's 200 response omits config, which the generated model requires:

response attribute keys: [author, created_at, experiment, is_auto_experiment, name, project_id, updated_at]
has config key? False

So it fails with missing field 'config', again after the experiment exists.

Fix

Both now use the raw client, which is what every other llm-obs command in this file already does — the typed client was the outlier here. Route, request body and required fields are unchanged, so the API contract is untouched; only the handling of a valid response changes. update prints {"experiment_id": ..., "status": "updated"} when the body is empty rather than a bare null.

Also fixes the underlying cause in raw_client::parse_response_json, which had the same intolerance: an empty body is valid on a successful response (204, or 200 with no payload) and now yields JSON null instead of an EOF error. That helper is shared by every raw_* function, so a 204 from any endpoint no longer surfaces as a parse failure.

Verification

End to end against the live API:

command before after
experiments update exit 1, EOF while parsing a value exit 0, {"experiment_id": ..., "status": "updated"}, status change applied
experiments create exit 1, missing field 'config' exit 0, returns the new id

Tests added: empty-200 body, normal JSON body, a 500, and a create response missing config. Full suites pass — llm_obs 97, raw_client 23 (the latter covering the shared-parser change for regressions).

cargo fmt --check clean, cargo clippy --bin pup -- -D warnings clean. No Cargo.toml change, so the dependency set is untouched.

Note on the alternative

The stricter fix is upstream in datadog-api-client-rust — treat an empty 2xx as entity: None, or mark these responses optional in the OpenAPI spec it's generated from. That's the more correct layer but it's generated code in another repo; this change makes pup behave today without waiting on it, and stays consistent with how the rest of the file already calls these endpoints.

🤖 Generated with Claude Code

@tillwf
tillwf requested a review from a team as a code owner July 27, 2026 17:34
tillwf added a commit to tillwf/agent-skills that referenced this pull request Jul 27, 2026
…he pending fix

The exit-code warning said pup's experiment writes "fail while deserializing the API's response"
without saying why, which makes it impossible to tell whether an installed build is affected.
Both causes are now stated, each confirmed against the live API:

  * `experiments update` — a successful PATCH answers HTTP 200 with a ZERO-BYTE body, which the
    generated typed client hands to serde_json::from_str, failing with "EOF while parsing a value".
  * `experiments create` — the 200 response omits `config`, a field the generated model requires,
    giving "missing field config".

Neither is a request failure. In one run this fired four times and all four writes had applied.

Also records that DataDog/pup#682 fixes both, by routing these two writes through pup's raw client
(as every other llm-obs command already does) and making parse_response_json treat an empty
successful body as JSON null. With that build update exits 0 and prints
{"experiment_id": ..., "status": "updated"}, and create exits 0 returning the new id.

That PR is OPEN, NOT MERGED, and the skill says so rather than describing unreleased behaviour as
current. The detection advice is deliberately not "check the version": run the command and compare
the exit code against a read-back, the same discipline the rest of this file uses — a version
number would not have caught the --help probe that reported records-all present on a binary that
lacked it.

The setup gate keeps read-back unconditionally, since it is correct on both builds and avoids
branching on which one is installed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
platinummonkey
platinummonkey previously approved these changes Jul 27, 2026
@gsvigruha

Copy link
Copy Markdown
Contributor

/merge

@gh-worker-devflow-routing-ef8351

gh-worker-devflow-routing-ef8351 Bot commented Jul 27, 2026

Copy link
Copy Markdown

View all feedbacks in Devflow UI.

2026-07-27 19:27:10 UTC ℹ️ Start processing command /merge


2026-07-27 19:27:11 UTC ❌ MergeQueue

PR already in the queue with status waiting

Both commands reported failure on requests that had already succeeded, because the generated typed
client rejects the API's actual responses.

`experiments update`: a successful PATCH to /api/v2/llm-obs/v1/experiments/{id} answers HTTP 200
with a ZERO-BYTE body (verified against the live API). The generated client deserializes any
non-error response into LLMObsExperimentResponse unconditionally, so serde_json::from_str::<T>("")
fails with "EOF while parsing a value, line 1 column 0" and the command exits 1 — after the write
has landed. Anything scripting pup would read that exit code as failure and retry, double-writing.

`experiments create`: the API's 200 response omits `config`, which the generated model requires, so
the typed call fails with "missing field `config`" — again after the experiment exists.

Both now use the raw client, as every other llm-obs command in this file already does; the typed
client was the outlier. Request shape, route and required fields are unchanged, so this is not a
behaviour change to the API contract — only to how a valid response is handled. Update prints
{"experiment_id": ..., "status": "updated"} when the body is empty, rather than a bare null.

Also fixes the underlying cause in raw_client::parse_response_json, which had the same intolerance:
an empty body is valid on a successful response (204, or 200 with no payload) and now yields JSON
null instead of an EOF error. That is shared by every raw_* helper, so a 204 from any endpoint no
longer surfaces as a parse failure.

Verified end to end against the live API: update exits 0 with the status change applied, create
exits 0 and returns the new id. Tests cover the empty-200 body, a normal JSON body, a 500, and a
create response missing `config`; the full llm_obs (97) and raw_client (23) suites pass.
@tillwf
tillwf force-pushed the till.wohlfarth/llm-obs-experiment-writes-raw branch from 268cc0c to f7a1837 Compare July 27, 2026 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants